Conversation
🦋 Changeset detectedLatest commit: 29220e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR introduces generic type parameters to RequestMiddleware, constraining middleware functions to specific journey action types, removes the middleware field from configuration types, and updates tests to validate typed middleware behavior with Accept-Language header assignments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 29220e2
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e/journey-app/main.ts (2)
44-54: Same optional chaining inconsistency.Line 48 uses
req.headers.append()without optional chaining while line 49 usesreq.headers?.set(). Apply the same consistency fix here.♻️ Proposed fix
case 'JOURNEY_TERMINATE': req.url.searchParams.set('end-session-middleware', 'end-session'); req.headers.append('x-end-session-middleware', 'end-session'); - req.headers?.set('Accept-Language', 'zz-ZZ'); + req.headers.set('Accept-Language', 'zz-ZZ'); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/main.ts` around lines 44 - 54, The middleware handling the 'JOURNEY_TERMINATE' action mixes direct and optional-chained header access (req.headers.append(...) vs req.headers?.set(...)); make them consistent by using the same presence-guard approach for headers in that switch case — update the block handling 'JOURNEY_TERMINATE' to call the same method access style (either both optional-chained or both direct) on req.headers (e.g., change req.headers.append to req.headers?.append or change req.headers?.set to req.headers.set) so both header manipulations use the identical pattern.
29-42: Inconsistent optional chaining onreq.headers.Lines 32-33 and 37-38 use
req.headers.append()without optional chaining, while lines 34 and 39 usereq.headers?.set()with optional chaining. This inconsistency suggests either the optional chaining is unnecessary or the non-optional calls could fail ifheaderswere ever undefined.♻️ Proposed fix for consistency
case 'JOURNEY_START': req.url.searchParams.set('start-authenticate-middleware', 'start-authentication'); req.headers.append('x-start-authenticate-middleware', 'start-authentication'); - req.headers?.set('Accept-Language', 'xx-XX'); + req.headers.set('Accept-Language', 'xx-XX'); break; case 'JOURNEY_NEXT': req.url.searchParams.set('authenticate-middleware', 'authentication'); req.headers.append('x-authenticate-middleware', 'authentication'); - req.headers?.set('Accept-Language', 'yy-YY'); + req.headers.set('Accept-Language', 'yy-YY'); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/main.ts` around lines 29 - 42, The code inconsistently uses optional chaining on req.headers (mixing req.headers.append(...) and req.headers?.set(...)) inside the middleware handling action.type 'JOURNEY_START' and 'JOURNEY_NEXT'; make it consistent by treating req.headers as non-null (remove the ?. calls) or defensively guarding all header operations with optional chaining or a null check. Update the four header operations (setting 'x-start-authenticate-middleware', 'x-authenticate-middleware', and 'Accept-Language') so they all use the same approach (either req.headers.append(...) / req.headers.set(...) everywhere or check if req.headers before calling append/set) to prevent runtime errors and keep behavior predictable.e2e/journey-suites/src/request-middleware.test.ts (1)
68-75: Redundant assertions for Accept-Language.Lines 69-71 checking
.not.toContain('en-US')are redundant since lines 73-75 assert the exact expected values. Ifxx-XX,yy-YY, andzz-ZZare correct, they inherently don't containen-US.Consider removing lines 69-71 to reduce test verbosity.
♻️ Proposed simplification
// Check that Accept-Language header was modified from default en-US locale and set to correct value in each middleware - expect(startHeader?.get('Accept-Language')).not.toContain('en-US'); - expect(nextHeader?.get('Accept-Language')).not.toContain('en-US'); - expect(endHeader?.get('Accept-Language')).not.toContain('en-US'); - expect(startHeader?.get('Accept-Language')).toBe('xx-XX'); expect(nextHeader?.get('Accept-Language')).toBe('yy-YY'); expect(endHeader?.get('Accept-Language')).toBe('zz-ZZ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-suites/src/request-middleware.test.ts` around lines 68 - 75, Remove the redundant "not.toContain('en-US')" assertions for Accept-Language by deleting the three checks that reference startHeader?.get('Accept-Language'), nextHeader?.get('Accept-Language'), and endHeader?.get('Accept-Language') which assert they do not contain 'en-US'; keep the existing exact-value assertions that expect 'xx-XX', 'yy-YY', and 'zz-ZZ' respectively so the test remains concise and still validates the header transformations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/journey-app/main.ts`:
- Around line 44-54: The middleware handling the 'JOURNEY_TERMINATE' action
mixes direct and optional-chained header access (req.headers.append(...) vs
req.headers?.set(...)); make them consistent by using the same presence-guard
approach for headers in that switch case — update the block handling
'JOURNEY_TERMINATE' to call the same method access style (either both
optional-chained or both direct) on req.headers (e.g., change req.headers.append
to req.headers?.append or change req.headers?.set to req.headers.set) so both
header manipulations use the identical pattern.
- Around line 29-42: The code inconsistently uses optional chaining on
req.headers (mixing req.headers.append(...) and req.headers?.set(...)) inside
the middleware handling action.type 'JOURNEY_START' and 'JOURNEY_NEXT'; make it
consistent by treating req.headers as non-null (remove the ?. calls) or
defensively guarding all header operations with optional chaining or a null
check. Update the four header operations (setting
'x-start-authenticate-middleware', 'x-authenticate-middleware', and
'Accept-Language') so they all use the same approach (either
req.headers.append(...) / req.headers.set(...) everywhere or check if
req.headers before calling append/set) to prevent runtime errors and keep
behavior predictable.
In `@e2e/journey-suites/src/request-middleware.test.ts`:
- Around line 68-75: Remove the redundant "not.toContain('en-US')" assertions
for Accept-Language by deleting the three checks that reference
startHeader?.get('Accept-Language'), nextHeader?.get('Accept-Language'), and
endHeader?.get('Accept-Language') which assert they do not contain 'en-US'; keep
the existing exact-value assertions that expect 'xx-XX', 'yy-YY', and 'zz-ZZ'
respectively so the test remains concise and still validates the header
transformations.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/hip-lands-call.mde2e/journey-app/main.tse2e/journey-suites/src/request-middleware.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/client.store.utils.tspackages/journey-client/src/lib/config.slice.test.tspackages/journey-client/src/lib/config.slice.tspackages/journey-client/src/lib/config.types.tspackages/journey-client/src/types.ts
💤 Files with no reviewable changes (3)
- packages/journey-client/src/lib/config.types.ts
- packages/journey-client/src/lib/config.slice.ts
- packages/journey-client/src/lib/config.slice.test.ts
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed 3111ec7 to https://ForgeRock.github.io/ping-javascript-sdk/pr-544/3111ec7b3412799a887c37f590217b0959d0a9a2 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/journey-client - 87.3 KB (new) ➖ No Changes➖ @forgerock/sdk-logger - 1.6 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (14.74%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #544 +/- ##
==========================================
- Coverage 18.79% 14.74% -4.06%
==========================================
Files 140 153 +13
Lines 27640 26262 -1378
Branches 980 1055 +75
==========================================
- Hits 5195 3872 -1323
+ Misses 22445 22390 -55
🚀 New features to boost your workflow:
|
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4815
Description
Summary by CodeRabbit
New Features
Bug Fixes
Chores